🧪 Make pytest notify us about future warnings #15620

webknjaz · 2024-11-07T13:09:59Z

In essence, this configures Python to turn any warnings emitted in runtime into errors[1]. This is the best practice that allows reacting to future deprecation announcements that are coming from the dependencies (direct, or transitive, or even CPython itself)[2].

The typical workflow looks like this:

If a dependency is updated an a warning is hit in tests, the deprecated thing should be replaced with newer APIs.
If a dependency is transitive or we have no control over it otherwise, the specific warning and a regex matching its message, plus the module reference (where possible) can be added to the list of temporary ignores in pytest.ini.
The list of temporary ignores should be reevaluated periodically, including when dependency re-pinning in lockfile is happening.

webknjaz · 2024-11-07T13:11:01Z

@fosterseth this is what I mentioned some time ago when looking at that rrule bug ^

In essence, this configures Python to turn any warnings emitted in runtime into errors[[1]]. This is the best practice that allows reacting to future deprecation announcements that are coming from the dependencies (direct, or transitive, or even CPython itself)[[2]]. The typical workflow looks like this: 1. If a dependency is updated an a warning is hit in tests, the deprecated thing should be replaced with newer APIs. 2. If a dependency is transitive or we have no control over it otherwise, the specific warning and a regex matching its message, plus the module reference (where possible) can be added to the list of temporary ignores in `pytest.ini`. 3. The list of temporary ignores should be reevaluated periodically, including when dependency re-pinning in lockfile is happening. [1]: https://docs.python.org/3/using/cmdline.html#cmdoption-W [2]: https://pytest-with-eric.com/configuration/pytest-ignore-warnings/

AlanCoding · 2024-11-08T20:48:34Z

pytest.ini

+  # FIXME: Use `open()` via a context manager
+  # FIXME: in `awx/main/utils/common.py` to close hanging file descriptors
+  # FIXME: and then delete the entry.
+  once:unclosed file <_io.BufferedWriter name='[^']+'>:ResourceWarning:awx.main.utils.common


This feels like re-arranging deck chairs on the Titanic. Great that your change gets us exposed to new warnings, but it will make dependency upgrades take longer, which already happen too infrequently, and it almost guarantees that none of these issues will be worked. And they really should be.

The methods here are fine, but I'd like to at least have at least a plan to knock out the borderline absurd ones in the list.

I skipped addressing them because that wasn't my goal. I wanted to show how to configure pytest properly. Fixing them can be planned for whenever tech debt is worked on.
I don't think that it causes more work, it just shifts the time when mandatory changes need to be made. Besides, you can always add more ignores if you want to postpone.

Is this PR, itself, not tech debt? And like I said, I'm fine to file followup, but "whenever tech debt is worked on" doesn't happen. Even a catch-all issue referencing your list will never get attention.

I meant it to comment on the warning about hanging file descriptors because absolutely nobody will disagree with that change, for example:

awx/awx/asgi.py

Lines 27 to 28 in 69baa73

fd = open("/var/lib/awx/.tower_version", "r")

if fd.read().strip() != tower_version:

I don't want to just have this added to a forever-ignore list, unless, for some reason I'm wrong about the complexity of it. Again, filing issues about the specific issues being ignored might be a workable approach.

@AlanCoding I see that, but this is as much as I'm willing to contribute right now. So merging it already would be more useful than waiting for months to see if I'll ever get to it.

This PR is useful in that it would prevent new similar problems from appearing silently over time. That's what I want out of it. This patch is not a tech debt, but rather documenting the existing issues that need to be addressed regardless of whether the strictness setting is set, though.

This doesn't just document existing issues, it changes the way of working by introducing an ignore-list. The reason the warnings were not silenced is because of a shared belief that they are important and should have code changes to resolve. This disposition doesn't necessarily change... but if you're not clear, people will do the wrong thing. I don't trust that without an example, so I created one:

webknjaz#2

Getting a new process for contributions, without getting some contributions (separate PRs is fine) is not something I'm really happy with. But here, I'm seeking an explicit consensus between you and me, and specific followup issues.

Okay, that makes sense in a way. Though, I don't think it changes the process. Previously, deprecation warnings would have to be addressed at some point and they were invisible (by default UserWarning instances are printed on the terminal, while many things, DeprecationWarning and FutureDeprecationWarning included never show up anywhere). To reveal those, one has to know upfront that they have to invoke everything using python -Werror -Im thing (or similar via an env var). And then, those places would have to be fixed. The problem is that with invisible warnings that accumulate over time, one would have to be aware of all these. And the process was fixing the errors too late when upstreams would replace warnings with hard failures.
The current setup makes these things visible ahead of time which is a pretty much standard best practice in the external Python world. So to me saying that the process changed would be too strong. I'd rather say that it became more transparent.

Remaining easily workable categories I see are:

FIXME: Delete this entry once USE_L10N use is removed

remove distutils from repo (only use, not dep)

Delete this entry once naive dates aren't passed to DB lookup

'index_together' is deprecated. Use 'Meta.indexes'

Using QuerySet.iterator.. after prefetch_related.. without specifying chunk_size

remove always-true assertions

fix Pagination may yield inconsistent results with an unordered object_list. .class

Channel's inbuilt http protocol AsgiHandler is deprecated. Use Django's get_asgi_application

Use codecs.open() via a context manager

I would like these to be filed, so @djyasin can have the available for the next sprint. I would have liked to have them even for story pointing, but it might be too late for the current round.

@AlanCoding fair. I was also postponing fixing things like open() under CM, since those are straightforward and could be a good learning opportunity for folks to try addressing them as opposed to more involved problems.

By the way, in the linked place in asgi, a more elegant solution would be pathlib.Path('/var/lib/awx/.tower_version').read_text().strip() even. This is to say that every place needs to be inspected additionally, beyond the generic suggestion I've put in the comments.

sonarcloud · 2024-11-11T15:23:03Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fosterseth · 2024-11-11T20:31:46Z

pytest.ini

+  # FIXME: Use `open()` via a context manager
+  # FIXME: in `awx/main/tests/unit/test_tasks.py` to close hanging file
+  # FIXME: descriptors and then delete the entry.
+  once:unclosed file <_io.TextIOWrapper name='[^']+' mode='r' encoding='UTF-8'>:ResourceWarning:awx.main.tests.unit.test_tasks


so this line can be removed with @AlanCoding 's commit?

@fosterseth I think he removed one entry already, I'm not sure about others. There were several different warnings in some files. Somebody needs to check it.

webknjaz · 2024-11-11T21:03:44Z

awx/main/tests/unit/test_tasks.py

+        with open(config_loc, 'r') as f:
+            shade_config = f.read()


@AlanCoding here's an example of what I meant @ #15620 (comment)

Suggested change

with open(config_loc, 'r') as f:

shade_config = f.read()

shade_config = Path(config_loc).read_text()

webknjaz · 2024-11-11T21:06:54Z

awx/main/utils/common.py

+    def tmp_write(path, data):
+        with open(path, 'wb') as f:
+            f.write(data)
+
+    threading.Thread(target=tmp_write, args=(path, data)).start()


@AlanCoding another example of #15620 (comment):

Suggested change

def tmp_write(path, data):

with open(path, 'wb') as f:

f.write(data)

threading.Thread(target=tmp_write, args=(path, data)).start()

threading.Thread(target=lambda p, d: Path(p).write_bytes(d), args=(path, data)).start()

This is one of the reasons I didn't really want to include fixing things into the scope of the PR, since a good solution might need to be iterated on, which may make the scope grow indefinitely.

That's fine with me. I suspected Path could do this more clearly. It's your PR, you can include it or not.

I didn't exactly expect you to merge it. I could have changed the base after your PR.

github-actions bot added the component:cli label Nov 7, 2024

webknjaz force-pushed the maintenance/pytest-filterwarnings branch 6 times, most recently from 3572b5b to 96cf287 Compare November 7, 2024 15:09

github-actions bot added the component:awx_collection issues related to the collection for controlling AWX label Nov 7, 2024

webknjaz force-pushed the maintenance/pytest-filterwarnings branch from 1f96561 to 912a056 Compare November 7, 2024 22:03

AlanCoding reviewed Nov 8, 2024

View reviewed changes

Address unclosed fd warnings

9cf4af5

github-actions bot added the component:api label Nov 11, 2024

AlanCoding approved these changes Nov 11, 2024

View reviewed changes

fosterseth reviewed Nov 11, 2024

View reviewed changes

webknjaz commented Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧪 Make pytest notify us about future warnings #15620

🧪 Make pytest notify us about future warnings #15620

webknjaz commented Nov 7, 2024

webknjaz commented Nov 7, 2024

AlanCoding Nov 8, 2024

webknjaz Nov 8, 2024

AlanCoding Nov 11, 2024

webknjaz Nov 11, 2024 •

edited

Loading

AlanCoding Nov 11, 2024

webknjaz Nov 11, 2024

AlanCoding Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

sonarcloud bot commented Nov 11, 2024

fosterseth Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

AlanCoding Nov 12, 2024

AlanCoding Nov 12, 2024

	fd = open("/var/lib/awx/.tower_version", "r")
	if fd.read().strip() != tower_version:

	with open(config_loc, 'r') as f:
	shade_config = f.read()
	shade_config = Path(config_loc).read_text()

🧪 Make pytest notify us about future warnings #15620

Are you sure you want to change the base?

🧪 Make pytest notify us about future warnings #15620

Conversation

webknjaz commented Nov 7, 2024

webknjaz commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 11, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz Nov 11, 2024 •

edited

Loading